Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added option to disable structLogs for debug_trace* rpc methods. #1931

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Added option to disable structLogs for debug_trace* rpc methods. #1931

merged 2 commits into from
Oct 12, 2023

Conversation

ohijiho
Copy link
Contributor

@ohijiho ohijiho commented Sep 25, 2023

Description

The easiest reliable way to get return data of a transaction is debug_traceTransaction rpc, but it comes with huge structLogs data, which uses a lot of network traffic.

This is a simple solution for that: Just add an option to disable it.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

  1. Start the chain
  2. Make a transaction
  3. Call trace_debugTransaction with disableLogs set to true and then false.
  4. Check that structLogs field is filled only when disableLogs is set to false.

Additional comments

I removed some test code lines that skips certain test suites. I don't know exactly why they have been made skipped originally, but they seems to work well now.

vcastellm
vcastellm approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vcastellm vcastellm requested a review from a team September 29, 2023 10:00
@vcastellm vcastellm self-assigned this Sep 29, 2023
@vcastellm vcastellm added the feature New update to Polygon Edge label Sep 29, 2023
@vcastellm vcastellm changed the title Added option to disable sturctLogs for debug_trace* rpc methods. Added option to disable structLogs for debug_trace* rpc methods. Sep 29, 2023
dusan-maksimovic
dusan-maksimovic approved these changes Oct 2, 2023
View reviewed changes
Copy link
Contributor

@dusan-maksimovic dusan-maksimovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Perhaps we should align all the flags in TraceConfig so we don't have half of them be EnableXYZ and the other half DisableXYZ. But this can be done through some other PR.

@Stefan-Ethernal
Copy link
Collaborator

Stefan-Ethernal commented Oct 2, 2023 •
edited
Loading

so we don't have half of them be EnableXYZ and the other half DisableXYZ

@ohijiho I agree entirely with @dusan-maksimovic here. Also then it makes more sense to expose the EnableMemory, EnableStorage, and EnableStack fields, and no need to have DisableStructLogs in general. WDYT?

@ohijiho
Copy link
Contributor Author

ohijiho commented Oct 2, 2023

so we don't have half of them be EnableXYZ and the other half DisableXYZ

@ohijiho I agree entirely with @dusan-maksimovic here. Also then it makes more sense to expose the EnableMemory, EnableStorage, and EnableStack fields, and no need to have DisableStructLogs in general. WDYT?

Then, structLogs is disabled by default and enabled when one of memory, storage or stack is enabled? If that's the case, the config will be restricted so that there's no option to enable only the structLogs.

FYI, the main purpose of this PR is adding an option to entirely disable the structLogs, which is different from disabling all three extra informations.

@Stefan-Ethernal Stefan-Ethernal merged commit e0d2891 into 0xPolygon:develop Oct 12, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dusan-maksimovic dusan-maksimovic dusan-maksimovic approved these changes

@Stefan-Ethernal Stefan-Ethernal Stefan-Ethernal approved these changes

@vcastellm vcastellm vcastellm approved these changes

Assignees

@vcastellm vcastellm

Labels
feature New update to Polygon Edge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ohijiho @Stefan-Ethernal @vcastellm @dusan-maksimovic